-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Node ESM #3630
Support Node ESM #3630
Conversation
Build successful! 🎉 |
Build successful! 🎉 |
@snowystinger Thanks for doing this! BTW on my local patch-package, this is what I did: diff --git a/dist/module.js b/dist/module.mjs
similarity index 99%
rename from dist/module.js
rename to dist/module.mjs
index 363c04eaa6f97ac44194025a6456829d02bc1210..6c0f9b3a6704d4b3507bd4945a0a21527d0a09fb 100644
--- a/dist/module.js
+++ b/dist/module.mjs
@@ -695,4 +695,4 @@ function $fdbe26a36ce1c672$export$736bf165441b18c7() {
export {$fd933927dbac1f15$export$46d604dce8bf8724 as shouldKeepSpectrumClassNames, $fd933927dbac1f15$export$f9d3bfd10703eb31 as keepSpectrumClassNames, $fd933927dbac1f15$export$ce4ab0c55987d1ff as classNames, $bde65b0159e7c06e$export$a5f5a6912b18861c as getWrappedElement, $3df547e395c4522f$export$32d5543ab307c01 as useMediaQuery, $98e5a8ae0e6415af$export$a5795cc979dfae80 as createDOMRef, $98e5a8ae0e6415af$export$79d69eee6ae4b329 as createFocusableRef, $98e5a8ae0e6415af$export$c2c55ef9111cafd8 as useDOMRef, $98e5a8ae0e6415af$export$96a734597687c040 as useFocusableRef, $98e5a8ae0e6415af$export$c7e28c72a4823176 as unwrapDOMRef, $98e5a8ae0e6415af$export$1d5cc31d9d8df817 as useUnwrapDOMRef, $380ed8f3903c3931$export$fe9c6e915565b4e8 as baseStyleProps, $380ed8f3903c3931$export$e0705d1a55f297c as viewStyleProps, $380ed8f3903c3931$export$abc24f5b99744ea6 as dimensionValue, $380ed8f3903c3931$export$f348bec194f2e6b5 as responsiveDimensionValue, $380ed8f3903c3931$export$f3c39bb9534218d0 as convertStyleProps, $380ed8f3903c3931$export$b8e6fb9d2dff3f41 as useStyleProps, $380ed8f3903c3931$export$46b6c81d11d2c30a as passthroughStyle, $380ed8f3903c3931$export$52dbfdbe1b2c3541 as getResponsiveProp, $59d09bcc83651bf9$export$1e5c9e6e4e15efe3 as useSlotProps, $59d09bcc83651bf9$export$365cf34cda9978e2 as cssModuleToSlots, $59d09bcc83651bf9$export$8107b24b91795686 as SlotProvider, $59d09bcc83651bf9$export$ceb145244332b7a2 as ClearSlots, $54cda195bd4173fb$export$e52e2242b6d0f1d4 as useHasChild, $fdbe26a36ce1c672$export$736bf165441b18c7 as useIsMobileDevice, $857d64dbfd73d664$re_export$useValueEffect as useValueEffect, $1051245f87c5981d$export$8214320346cf5104 as BreakpointProvider, $1051245f87c5981d$export$140ae7baa51cca23 as useMatchedBreakpoints, $1051245f87c5981d$export$199d6754bdf4e1e3 as useBreakpoint, $857d64dbfd73d664$re_export$useResizeObserver as useResizeObserver};
-//# sourceMappingURL=module.js.map
+//# sourceMappingURL=module.mjs.map
diff --git a/dist/module.js.map b/dist/module.mjs.map
similarity index 100%
rename from dist/module.js.map
rename to dist/module.mjs.map
diff --git a/package.json b/package.json
index 7afca4093e0b4bcecc0d9a10b9639d7e3ce8dad4..85f3bd6e0b20c3b16651e38fc37215b39dcacc9e 100644
--- a/package.json
+++ b/package.json
@@ -4,7 +4,11 @@
"description": "Spectrum UI components in React",
"license": "Apache-2.0",
"main": "dist/main.js",
- "module": "dist/module.js",
+ "module": "dist/module.mjs",
+ "exports": {
+ "import": "./dist/module.mjs",
+ "require": "./dist/main.js"
+ },
"types": "dist/types.d.ts",
"source": "src/index.ts",
"files": [ By making the module file end with |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the supportESM.js seems to be the script made to update all the package.json files and is around to run in case we forget to manually do this on new packages in the future, right?
meta: { | ||
fixable: 'code' | ||
}, | ||
create: function (context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no actual change to this logic, it's just indented one level and wrapped differently to match new eslint requirements
@@ -11,7 +11,7 @@ | |||
*/ | |||
|
|||
import {Badge} from '../'; | |||
import CheckmarkCircle from '@spectrum-icons/workflow/src/CheckmarkCircle'; | |||
import CheckmarkCircle from '@spectrum-icons/workflow/CheckmarkCircle'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected all instance of imports like this, we should never import icons from the src file
@@ -34,7 +34,7 @@ function template(iconName) { | |||
let jsx = compileSVG(path.join(expressDir, expressName + '.svg'), 'ExpressIcon'); | |||
|
|||
return ( | |||
`import {${iconName} as IconComponent} from '@adobe/react-spectrum-ui/dist/${iconName}'; | |||
`import {${iconName} as IconComponent} from '@adobe/react-spectrum-ui/dist/${iconName}.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adobe/react-spectrum-ui only provides CJS, so we include the file extension to force any loaders to recognize that as opposed to everywhere else where we let the loader choose which one it wants to use
}, | ||
}); | ||
|
||
export async function resolve(specifier, context, next) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a fake css loader, it's only needed for our esm test build, it does not get used anywhere else
Build successful! 🎉 |
Build successful! 🎉 |
@@ -7,6 +7,11 @@ | |||
"main": "dist/main.js", | |||
"module": "dist/module.js", | |||
"types": "dist/types.d.ts", | |||
"exports": { | |||
"types": "dist/types.d.ts", | |||
"import": "dist/module.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thing: these should be
"import": "dist/module.js", | |
"import": "dist/module.mjs", |
right? Same issue for the other plop templates if so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, yep, i'll do here so we can back out if we need to easily
Build successful! 🎉 |
Build successful! 🎉 |
Congrats @snowystinger ! What an amazing feat to pull this off! |
Adding an update to reduce the size of some of our files |
#3810 has been merged, it'll be in the nightlies starting tonight |
This reverts commit 5198442. # Conflicts: # yarn.lock
Unfortunately, we had to revert this for now. It broke webpack 4, which still has more downloads per week than webpack 5. See #3839 for details. We may be able to regroup in the new year, but so far we haven't found a solution that works across all tools. |
@devongovett Any chance this is getting picked up again? Not using .mjs for the esm bundle (as per spec) is causing issues as you can read in the mention above. |
Yes, this is still on our radar and something we'd like to address. We're accepting contributions and ideas on how to address the webpack 4 issue. It may be a sprint or two before we get back to it. |
I read up on the issue in #3839. Seems like the exact same thing I'm running into now with Next.js (and webpack 5). Unfortunately there is only one way forward and that's to use .mjs extensions. This fixes it for me in Next.js. Not an ideal solution, but it might be necessary for webpack4 users to add a custom config moving forward. module.exports = {
transpilePackages: ['@saas-ui/date-picker'],
// optional
webpack(config, options) {
config.module.rules.push({
type: 'javascript/auto',
test: /\.mjs$/,
use: [options.defaultLoaders.babel],
})
return config
}
} |
I think I got a new approach working in #4038 |
Closes #2881
I've updated the NextJS test app to use ESM. There is a comment on a commit 5dd5f72#comments that was built against our local packages in the branch. This should be sufficient to say that we support ESM now with the other changes in this PR.
I've included a circleci test to run against all our built packages with esm
This is old, I'm leaving it in to explain some things we considered
This brought up some other issues in order to get it working. All our icon imports needed to have the '.js' suffix and also needed to be imported correctly from packages and not direct from 'src' directories.I needed to update node so I could write a CSS loader for NodeJS. This resulted in some tests getting updated because the Intl formatting was updated by Node. I've matched it against current Chrome implementation and it matches.
It also required me to make it ok in the linter for package circular dependencies where the circle is just itself, this is seen in react-aria/i18n, it'll be checked in the verdaccio step, though may cause some issues for yarn 2 or pnpm.
BLOCKED BY #3706
✅ Pull Request Checklist:
📝 Test Instructions:
Test apps generated with published packages, last time they were built is on this commit 5dd5f72#comments
commits after that were unrelated and should not affect this
Test apps generated in normal build comment
You can also verify these locally, if you're running the esm test, then you must be on node > 16.18. Though the above comments should take care of anything you'd do locally.
🧢 Your Project: